Skip to content

pebble: panic on incorrect strict MemFS methods use#3098

Merged
pav-kv merged 3 commits into
cockroachdb:masterfrom
pav-kv:fix-deadlock-in-memfs
Nov 28, 2023
Merged

pebble: panic on incorrect strict MemFS methods use#3098
pav-kv merged 3 commits into
cockroachdb:masterfrom
pav-kv:fix-deadlock-in-memfs

Conversation

@pav-kv

@pav-kv pav-kv commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Strict MemFS methods can be used by mistake on a non-strict MemFS. Panic if this happens, instead of (previously) ignoring this silently. This saves a test writer time being confused if they mis-use the type.

Before this fix, if someone calls SetIgnoreSyncs() on a non-strict
MemFS, it does not unlock the mutex before returning. This causes all
other methods of MemFS to deadlock.
Strict MemFS methods can be used by mistake on a non-strict MemFS. Panic
if this happens, instead of (previously) ignoring this silently. This
saves a test writer time being confused if they mis-use the type.
@pav-kv pav-kv requested a review from jbowens November 22, 2023 17:37
@cockroach-teamcity

Copy link
Copy Markdown
Member

This change is Reviewable

@pav-kv

pav-kv commented Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Review commit by commit is recommended.

@pav-kv

pav-kv commented Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

@jbowens PTAL

@pav-kv

pav-kv commented Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

There is one test failing on Windows, is this an unrelated flake?

=== RUN   TestMetricsWithSync
    log_writer_test.go:452: 
        	Error Trace:	D:\a\pebble\pebble\record\log_writer_test.go:452
        	Error:      	"5000" is not less than or equal to "0"
        	Test:       	TestMetricsWithSync
--- FAIL: TestMetricsWithSync (0.03s)

@jbowens jbowens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the Windows CI failure appears to be #1877 an unsquashed flake.

Should we instead expose a StrictMemFS type so that the type system prevents calling these methods? We could just export the methods on the MemFS, and expose a new StrictMemFS type that embeds the MemFS and exports the methods.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @pavelkalinnikov)

@pav-kv

pav-kv commented Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

Good idea adding a new type, I can do in a follow-up.

Alternatively, we may choose to make MemFS strict by default, because the performance problems preventing this can be fixed by #3104.

@pav-kv pav-kv merged commit 47104ed into cockroachdb:master Nov 28, 2023
@pav-kv pav-kv deleted the fix-deadlock-in-memfs branch November 28, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants